You should remove the not useful comment at the end of line here.

ha, i've had a remark on this one that I should catch the error. Should I catch, or will it be correctly handled by calling code ?

To be fully safe, you can initialize @ordered_script_list to empty list, run an eval where you want to populate the list, you should even move the closedir(DIR) from l.99 in the eval. Then if something goes wrong, the list will be kept empty or even partially filled. If the list remains empty, the while will do nothing and no custom property will be inventoried, but the Rudder module will still finish and inventory other informations.

By convention, don't assume $logger is defined while it was passed in %params. So add a if $logger at the end of line before ;

ha, this is something I was not aware of

The same here, don't assume $logger is defined

I think you missed an i to say properties in @custom_propertes_list

indeed:)

Even if JSON::PP is still necesarry for some FusionInventory agent modules, I would advise you to better depend on UNIVERSAL::require and add a JSON::PP->require() later in the related-eval. This will delay the load until it is really needed and even will save some memory usage while no custom script is found.

i mimicked what was in Inventory/Virtualization/Docker.pm and HTTP/Client/Fusion.pm
i'll try the proposed solution

I know you want here to not use runCommand() API as it won't fail if script fails as you asked us about that on IRC. But it is important then to at least add a debug2 level logging just before as is doing runCommand() so we can debug easily something is started from here and what.
Another point, does this scripts need to be run as root user ? This can look like a security hole if somebody other than root gains the privilege to write a script there.

I can add the logger; however i'm not sure which user could be used to run the scripts, except root (or current admin user).
Plus, if someone can access as root to /var/rudder, we have other issues on our hands. @peckpeck do you have any advices on this ?

today it is run by root, this means we have to make sure that this directory is not writable by anyone except root.
For users not having rudder, /var is writable only by root, so this is not a security problem for them

Okay, then I'll be okay if you add a simple check to see if scripts is owned and executable by root. You should then log a warning and not start the script if it is not owned and runnable by admin.

More we add safe controls, more we can be sure nothing goes wrong for users.

Is your remarks also valid for win32 platform ? This is the most sensible...

yes, c:\Program Files\Rudder is also owned and readable only by Administrator

By root or by the current user

if it is not runable, it cannot be run by the command, so the check is not interesting.
If you want a better check, you can check that when you are root, it is not writable by anyone else.

here, I failed to check if file was owned by root (or its Windows equivalent) - as there can be several admin
how would you do that ? @peckpeck @g-bougard

I used
my $stats = stat($script_file);
my $owner = $stats->uid;
my $currentUser = $<;

file must be owned by root or current user
if (($owner != 0) && ($owner != $currentUser)) {
$logger->error("Skipping script $script_file as it is not owned by root nor current user (owner is $owner)") if $logger;
next;
}
but this fails miserably on Windows - it always believe the owner of the file is 0

this fails on windows - even if file is writable only by admin, it gets 777 :(